Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues around component classes / (text) opacity #3415

Closed
wants to merge 15 commits into from

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented Sep 27, 2023

This PR will fix #3413, #3342, and #3304.

This started out as a PR that intended to fix an issue with the option list and kept growing in size as I kept uncovering more issues with component classes & styles, (partial) rich styles, and interactions with (text-)opacity styles.

I've stopped digging because there's some things with component classes we'd like to iron out and those will probably help with what's not handled in this PR yet.

Some things that were left to be done/fixed/considered:

The invalid CSS will throw off the TCSS VS Code extension and the whole file ends up looking very odd.
Now that text opacity is taken into account in the calculation of rich_style, the text color of disabled tabs became too light. To compensate for it, the text opacity in Tab:disabled could be set to 71% and that would match the previous value. However, 71% feels like a terrible value to have in CSS so I opted to go with 70% (which looks ALMOST EXACTLY THE SAME) and then I updated the snapshot test.
This snapshot will be used to make sure that the opacity is being taken into account when rendering widgets. This shows that some of the issues in #3304 and #3413.
@rodrigogiraoserrao
Copy link
Contributor Author

de6a479 introduces a snapshot test app that you can run with textual run tests/snapshot_tests/snapshot_apps/component_classes_opacity.py.

The “empty boxes” are a good thing because I'm using component classes to set colours to transparent.
The screenshot shows that the only widgets that aren't fixed are DirectoryTree, Tree, Checkbox, and RadioButton (i.e., we need to fix Tree and ToggleButton).

Screenshot 2023-09-27 at 17 43 13

The screenshot shows that the previous commits fix #3304.

@rodrigogiraoserrao rodrigogiraoserrao linked an issue Sep 27, 2023 that may be closed by this pull request
@rodrigogiraoserrao
Copy link
Contributor Author

The reason the (directory) tree and toggle button labels are still white is because they use partial rich styles to enable markup to be used in the labels, which don't currently take into account text opacity.

To know how to resolve this, we need to know what should happen if a widget has mark up and conflicting TCSS.
For example, what should the checkbox Checkbox("[red bold]This is just[/] some text.") look like if the following TCSS is specified:

Checkbox > .toggle--label {
    color: blue;
    text-opacity: 50%;
}
  1. The red/bold remains, but faded 50% and the “some text” is blue 50%
  2. The red/bold remains and not faded, “some text” is blue 50%
  3. The bold remains, the whole text is blue 50%
  4. The TCSS overrides the markup completely and everything is just blue 50%
  5. The existence of markup overrides the TCSS completely and the TCSS is essentially ignored
  6. Some other combination I'm not considering

After I have the answer to this question, I can get to fixing the code so that it complies with what we decide.

The component classes themselves are applied on top of successive widgets that have non-opaque backgrounds, we need to use the computed final background color after taking into account the transparency of the ancestors.
@rodrigogiraoserrao rodrigogiraoserrao linked an issue Oct 4, 2023 that may be closed by this pull request
This snapshot test will only be enforced after #3464 is fixed, at which point the snapshot test should be similar to test_component_classes_opacity, but where each sentence is slightly visible in a faded shade or red.
text-opacity: 50%;
text-opacity: 70%;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accounts for the now-correct calculation of the final text color when opacity is present.

@willmcgugan
Copy link
Collaborator

I have an alternative solution in mind for _redirect_component_styles. Solution is good, but I want to make this change as part of a larger update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants